Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Give temporaries in if let guards correct scopes #119122

Merged
merged 2 commits into from
Dec 25, 2023
Merged

Give temporaries in if let guards correct scopes #119122

merged 2 commits into from
Dec 25, 2023

Conversation

matthewjasper
Copy link
Contributor

Temporaries in if-let guards have scopes that escape the match arm, this causes problems because the drops might be for temporaries that are not storage live. This PR changes the scope of temporaries in if-let guards to be limited to the arm:

_ if let Some(s) = std::convert::identity(&Some(String::new())) => {}
//                Temporary for Some(String::new()) is dropped here ^

We also now deduplicate temporaries between copies of the guard created for or-patterns:

// Only create a single Some(String::new()) temporary variable
_ | _ if let Some(s) = std::convert::identity(&Some(String::new())) => {}

This changes MIR building to pass around ExprIds rather than Exprs so that we have a way to index different expressions.

cc #51114
Closes #116079

@rustbot
Copy link
Collaborator

rustbot commented Dec 19, 2023

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 19, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@TaKO8Ki TaKO8Ki added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 20, 2023
@matthewjasper
Give temporaries in if let guards correct scopes
d437a11 
- Make temporaries in if-let guards be the same variable in MIR when
  the guard is duplicated due to or-patterns.
- Change the "destruction scope" for match arms to be the arm scope rather
  than the arm body scope.
- Add tests.
@matthewjasper matthewjasper added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 22, 2023
@TaKO8Ki
Copy link
Member

TaKO8Ki commented Dec 25, 2023

@bors rollup=never

@TaKO8Ki
Copy link
Member

TaKO8Ki commented Dec 25, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Dec 25, 2023

📌 Commit d437a11 has been approved by TaKO8Ki

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 25, 2023
@bors
Copy link
Contributor

bors commented Dec 25, 2023

⌛ Testing commit d437a11 with merge f2348fb...

@bors
Copy link
Contributor

bors commented Dec 25, 2023

☀️ Test successful - checks-actions
Approved by: TaKO8Ki
Pushing f2348fb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 25, 2023
@bors bors merged commit f2348fb into rust-lang:master Dec 25, 2023
12 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Dec 25, 2023
@bors bors mentioned this pull request Dec 25, 2023
Closed
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f2348fb): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.9% [2.4%, 3.4%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.0% [-2.6%, -1.6%] 3
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.3% [0.3%, 0.3%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -0.0% [-0.0%, -0.0%] 3
All ❌✅ (primary) 0.3% [0.3%, 0.3%] 1

Bootstrap: 672.006s -> 669.882s (-0.32%)
Artifact size: 312.63 MiB -> 312.63 MiB (-0.00%)

@matthewjasper matthewjasper deleted the if-let-guard-scoping branch January 3, 2024 14:43
@matthewjasper matthewjasper added the F-if_let_guard `#![feature(if_let_guard)]` label Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@TaKO8Ki TaKO8Ki

Labels
F-if_let_guard `#![feature(if_let_guard)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.77.0
Development

Successfully merging this pull request may close these issues.

ICE: if_let_guard + mir drop tracking: broken mir
6 participants
@matthewjasper @rustbot @rust-log-analyzer @TaKO8Ki @bors @rust-timer